- 
                Notifications
    
You must be signed in to change notification settings  - Fork 11
 
Add middlewares management #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
namedtuples are immutables, but we must allow edition of commands and results inside the middleware, so we use real objects, but wuth __slots__ instead of __dict__, which is faster and much memory efficient
Conflicts: limpyd/fields.py run_tests.py
| 
           Missing docs, but what about using my loooong PR description ?  | 
    
| 
           Yes, for the doc :)  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As usual, I don't feel comfortable when changing the method behaviour from a parameter. IMHO, here we need two methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I don't get the need of this direct call. Is there a constraint not to call a middleware from inside a command call ? Or is it specific to index calls? Need your lights here ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to decide: ALL redis commands are passed to the middleware, or ONLY ones issued by the user.
Because when we manage indexes, we have mainly direct calls to redis (sadd...), but also some proxy_get, and both should be managed the same way for consistency
NB1: btw i knew you wouldn't like this update, it was here to initiate the conversation ;)
NB2: i'll merge master into this branch later, when done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm, not easy.
Except for performance reasons, I think we need a very generic hook, so at this point my preference goes to calling middlewares for every calls from Limpyd.
About performance, I don't know what can be the cost of calling the middlewares hook for every call.
| 
               | 
          ||
| for middleware in self.prepared_middlewares['pre_command']: | ||
| result = middleware.pre_command(command, context) | ||
| if result: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if result is not None ?
| # _models keep an entry for each defined model on this database | ||
| self._models = dict() | ||
| 
               | 
          ||
| if middlewares is not None: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the check is not None is not necessary
| self._has_scripting = False | ||
| return self._has_scripting | ||
| 
               | 
          ||
| @property | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use @cached_property
The goal of this middleware system is to add an entry point to help enhance the limpyd behavior without touching its core.
A middleware is a class which surround a redis call when a command n a field is called.
The
pre_commandmethod of each middleware, is called before the redis call, then thepost_commandmethod of each middleware in reverse order.If a
pre_commandmethod returns something (notNone), the redis call won't be done and the result value will be used instead (and otherpre_commandmethods won't be called too).A
post_commandmethod must return aResultobject, which contains thevaluegot from redis (or from apre_commandmethod).A
pre_commandmethod accepts two arguments:command, aCommandobject, with three fields (name(of the command) andargsandkwargsto pass to the command)context, adictinitialized with asenderentry, which is the object on which the original command apply (a field or a model)The
commandobject can be modified if wanted, before used to call redis.The
contextdict can also be modified, to add properties useful to the middleware that it would need in thepost_commandfor example. Note that each called middleware share the samecontextdictionary.A
post_commandmethod accepts three arguments:command, theCommandobject passed through thepre_commandmethodsresult, aResultobject with a single field,value, which hold the result of the redis callcontext, the same dictionary passed through thepre_commandmethods.The
resultcan be modified, and it's value returned from the lastpost_commandmetho will be used as a result to the call. (Thecommandandcontextobjects can still be modified, but it is not really useful)To define a new middleware, simply inherit from
BaseMiddlewareinlimpyd.middlewaresand define apre_commandand/or apost_command(they are both optional, only the existing methods will be used).To use middlewares, you must declare it when creating your database:
database = RedisDatabase( middlewares=[ AMiddleware(with_some=params), AnotherMiddleware(), ], **my_connection_settings )An example (but usable) middleware is provided in
limpyd.middlewares, to log commands, their results (optional) and the duration (optional too):LoggingMiddleware. It can log stuff like: